Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add author column #129

Merged
merged 9 commits into from
Sep 27, 2023
Merged

feat: add author column #129

merged 9 commits into from
Sep 27, 2023

Conversation

Okabe-Junya
Copy link
Contributor

@Okabe-Junya Okabe-Junya commented Sep 21, 2023 •
edited by zkoppert
Loading

close #128

Proposed Changes

I have added an author column to each issue. As seen in the original issue and this simple sample, it now displays the user who created the issue. In the original issue, there is an @, but I decided not to include it as it could potentially trigger numerous notifications due to mentions.

| Title | URL | Author | Time to first response | Time to close | Time to answer | Time spent in waiting-for-review | Time spent in waiting-for-manager |
| --- | --- | --- | --- | --- | --- | --- | --- |
| Monthly issue metrics report | https://github.com/Okabe-Junya/sandbox/issues/249 | github-actions[bot] | None | None | None | None | None |

Readiness Checklist

  • The attributes of the IssueWithMetrics class have been modified. Therefore, this PR involves somewhat extensive changes.
  • Obtaining the author of the discussion was challenging, so it is not included in this PR (it is set to None).
  • The author attributes are mandatory, not optional. Would it be better if they were optional??
  • For the sample users in the tests, I am using alice, bob, and carol; is this appropriate?

Author/Contributor

  • If documentation is needed for this change, has that been included in this pull request
  • run make lint and fix any issues that you have introduced
  • run make test and ensure you have test coverage for the lines you are introducing
  • Label as either bug, documentation, enhancement, infrastructure, or breaking

@Okabe-Junya Okabe-Junya marked this pull request as draft September 21, 2023 18:18
@Okabe-Junya
Copy link
Contributor Author

Okabe-Junya commented Sep 21, 2023 •
edited
Loading

The tests pass, but it seems like make test is not working properly.

make test result

$ make test
pytest -v --cov=. --cov-config=.coveragerc --cov-fail-under=80 --cov-report term-missing
ERROR: usage: pytest [options] [file_or_dir] [file_or_dir] [...]
pytest: error: unrecognized arguments: --cov=. --cov-config=.coveragerc --cov-fail-under=80 --cov-report
  inifile: None
  rootdir: /Users/junyaokabe/workspace/issue-metrics

make: *** [test] Error 4

pytest result

$ pytest
======================================================================== test session starts =========================================================================
platform darwin -- Python 3.11.0, pytest-7.4.0, pluggy-1.2.0
rootdir: /Users/junyaokabe/workspace/issue-metrics
collected 49 items                                                                                                                                                   

test_discussions.py ..                                                                                                                                         [  4%]
test_issue_metrics.py .........                                                                                                                                [ 22%]
test_json_writer.py .                                                                                                                                          [ 24%]
test_labels.py ....                                                                                                                                            [ 32%]
test_markdown_writer.py ....                                                                                                                                   [ 40%]
test_time_to_answer.py ......                                                                                                                                  [ 53%]
test_time_to_close.py .....                                                                                                                                    [ 63%]
test_time_to_first_response.py ............                                                                                                                    [ 87%]
test_time_to_merge.py ...                                                                                                                                      [ 93%]
test_time_to_ready_for_review.py ...                                                                                                                           [100%]

========================================================================= 49 passed in 0.17s =========================================================================

@Okabe-Junya Okabe-Junya mentioned this pull request Sep 21, 2023
Closed
@Okabe-Junya Okabe-Junya mentioned this pull request Sep 21, 2023
Closed
@Okabe-Junya Okabe-Junya marked this pull request as ready for review September 21, 2023 18:49
@Okabe-Junya
Copy link
Contributor Author

The failure of make test was due to my oversight. Since I was able to measure the coverage, I will list the results here.

$ make test
pytest -v --cov=. --cov-config=.coveragerc --cov-fail-under=80 --cov-report term-missing
============================= test session starts ==============================
platform darwin -- Python 3.11.0, pytest-7.4.2, pluggy-1.2.0 -- /Users/junyaokabe/workspace/issue-metrics/venv/bin/python
cachedir: .pytest_cache
rootdir: /Users/junyaokabe/workspace/issue-metrics
plugins: cov-4.1.0
collecting ... collected 49 items

...

---------- coverage: platform darwin, python 3.11.0-final-0 ----------
Name                          Stmts   Miss  Cover   Missing
-----------------------------------------------------------
classes.py                        9      0   100%
discussions.py                   14      0   100%
issue_metrics.py                142     44    69%   62, 96-114, 129-132, 189, 204-205, 211, 214, 243, 280, 289, 296-304, 313-344, 358
json_writer.py                   29      3    90%   77, 123-124
labels.py                        58      5    91%   53, 69, 102, 107-108
markdown_writer.py               72      0   100%
time_to_answer.py                25      0   100%
time_to_close.py                 33      2    94%   48, 54
time_to_first_response.py        55      1    98%   102
time_to_merge.py                 12      0   100%
time_to_ready_for_review.py      11      0   100%
-----------------------------------------------------------
TOTAL                           460     55    88%

Required test coverage of 80% reached. Total coverage: 88.04%

============================== 49 passed in 0.26s ==============================

@zkoppert zkoppert added the enhancement New feature or request label Sep 22, 2023
@zkoppert
Copy link
Member

This looks great! Thanks for taking the time to add authors into the table.

I do think that we need to add a HIDE_AUTHOR configuration like the ones seen in the README. When putting these tables into an issue it can be hard to read wide table with lots of entries and that is why I think this is important to be configurable. Any thoughts on that?

@Okabe-Junya
Copy link
Contributor Author

Thank you for the thoughtful review and response.

I have implemented a feature to decide whether to display the author based on the environment variable HIDE_AUTHOR. It's working well in my environment.

It’s tough to find a good solution to the problem of the generated tables becoming long. I don't have a complete solution for this, but what do you think about the following ideas?

  • Hide some metrics by default.
  • Shorten the column names (in some columns, the column name seems to be the bottleneck).
  • Change the format of time display. (It might be a bit redundant when displaying time over one day in the ‘n days...’ format)

However, none of these are complete solutions, and it might be better not to include them in this PR.

@Okabe-Junya
Copy link
Contributor Author

Okabe-Junya commented Sep 22, 2023 •
edited
Loading

If it is undesirable for the table to be long, it might be good to set HIDE_AUTHOR to true by default (I will make corrections if needed.)

@Okabe-Junya Okabe-Junya mentioned this pull request Sep 22, 2023
Closed
zkoppert
zkoppert approved these changes Sep 27, 2023
View reviewed changes
Copy link
Member

@zkoppert zkoppert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@zkoppert
Copy link
Member

Thanks @Okabe-Junya for your continued contribution! I appreciate you taking the time to make this tool better for those that use it! 🎉 🚀

@zkoppert zkoppert merged commit c55c247 into github:main Sep 27, 2023
4 checks passed
@Okabe-Junya Okabe-Junya deleted the junya/feat/add-author branch September 27, 2023 18:01
@spier spier mentioned this pull request Oct 20, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zkoppert zkoppert zkoppert approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add author line for result in pull-requests
2 participants
@Okabe-Junya @zkoppert